Improve IBD documentation and IdentitySegments docs #3330
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3330 +/- ##
=======================================
Coverage 89.76% 89.76%
=======================================
Files 29 29
Lines 31292 31292
Branches 5738 5738
=======================================
Hits 28089 28089
Misses 1794 1794
Partials 1409 1409
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
I made the changes above and did some more here. I think it could of course be better but is good enough to go unless someone sees typos/errors. |
petrelharp
left a comment
petrelharp
left a comment
There was a problem hiding this comment.
These are all changes I implemented in my commit, but some of these comments have explanation so I'm putting them in still.
docs/ibd.md
Outdated
| segments returned are the longest possible ones: for a fixed pair | ||
| ``(u, v)`` we follow the ancestral paths from ``u`` and ``v`` up the | ||
| trees and merge together adjacent genomic intervals whenever both the | ||
| MRCA ``a`` and the full ancestral paths from ``u`` and ``v`` to ``a`` | ||
| are identical. |
There was a problem hiding this comment.
TODO: double-check this is true. I suspect this might need some nuance at least.
There was a problem hiding this comment.
None of the things I checked that might be "merged" are merged - even if I split an edge, that also splits the IBD segment.
docs/ibd.md
Outdated
| by genealogical path) the IBD segments are obtained by merging together | ||
| adjacent MRCA intervals that share the same ancestor and paths to that |
There was a problem hiding this comment.
| by genealogical path) the IBD segments are obtained by merging together | |
| adjacent MRCA intervals that share the same ancestor and paths to that | |
| by genealogical path) the IBD segments are those | |
| that share the same ancestor and paths to that |
docs/ibd.md
Outdated
| - Passing an empty list, e.g. ``within=[]``, is allowed and simply | ||
| yields a result with zero pairs and zero segments. |
There was a problem hiding this comment.
Unnecessary detail.
| - Passing an empty list, e.g. ``within=[]``, is allowed and simply | |
| yields a result with zero pairs and zero segments. |
docs/ibd.md
Outdated
| no greater than ``max_time`` are returned. The time is measured in | ||
| the same units as the node times in the tree sequence (e.g., generations). |
There was a problem hiding this comment.
What else would it be measured in?
| no greater than ``max_time`` are returned. The time is measured in | |
| the same units as the node times in the tree sequence (e.g., generations). | |
| no greater than ``max_time`` are returned. |
hyanwong
left a comment
hyanwong
left a comment
There was a problem hiding this comment.
I read it all through and LGTM
|
@benjeffery - looks like we're doing merges from main on this PR rather than rebases. Is there something we can do about this going forward to keep the linear history using the github UI? Or did you push to this branch manually @hyanwong ? |
|
I clicked the "rebase" button on Github before clicking "merge when ready", which seemed to be needed. Maybe that makes a branch on main to store the rebased version? |
|
There's an update branch button as well though, right? I think you updated by merging rather than rebase |
|
I'll see if I can disable the update branch button. |
|
I've tweaked the settings to not allow merge commits. |
|
The main branch is still linear though, as we had squash on. I've turned that off now as it wasn't supposed to be on. |
|
Great, thanks. Apologies if I pushed the wrong button |
4 participants
Extracted from #3329